Skip to content

ZA | 25-SDC-July | Luke Manyamazi | Sprint 3 | Implement Shell Tools Exercises #132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Luke-Manyamazi wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ZA | 25-SDC-July | Luke Manyamazi | Sprint 3 | Implement Shell Tools Exercises #132

Luke-Manyamazi wants to merge 5 commits into from

Conversation

@Luke-Manyamazi
Copy link

@Luke-Manyamazi Luke-Manyamazi commented Jul 30, 2025 •
edited
Loading

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

This PR includes implementations of basic Unix-like command-line tools in Node.js:

ls: Lists files in a directory.
cat: Reads and prints the contents of one or more files.
wc: Counts lines, words, and characters in one or more files, with support for flags (-l, -w, -c).

Each command was tested to match expected output behavior for the given use cases.

Questions

  1. Is my handling of default behavior in the wc command (i.e. when no flags are provided) aligned with how the actual Unix wc works?
  2. Do you recommend any improvements for structuring output formatting, especially when dealing with multiple files and totals?
  3. Are there any edge cases you suggest I test further?

@Luke-Manyamazi Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Jul 30, 2025
@Luke-Manyamazi Luke-Manyamazi linked an issue Jul 30, 2025 that may be closed by this pull request
01 Implement shell tools (cat, ls, wc) in JavaScript with NodeJS Luke-Manyamazi/My-Coursework-Planner#176
Open
@LonMcGregor LonMcGregor added the Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. label Aug 12, 2025
LonMcGregor
LonMcGregor reviewed Aug 12, 2025
View reviewed changes
Copy link

@LonMcGregor LonMcGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on this, I've left some comments for where you could start to improve more.

For your questions, yes, it looks like you're handling defaults for wc fine. I think you are handling output fine.

If you are thinking of handling more edge cases, think about how people might use these commands - what sort of arguments might get passed to your code?

implement-shell-tools/cat/cat.mjs Outdated
const lines = content.split('\n');

for (const line of lines) {
const shouldNumber = (numberAllLines && !numNotBlank) || (numNotBlank && line.trim() !== '');
Copy link

@LonMcGregor LonMcGregor Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not numbering non-blank lines when -b is used. Can you see why that is?

implement-shell-tools/ls/ls.mjs Outdated
.name("ls")
.description("Lists the files in a directory")
.option("-1, --one", "One per line")
.option("-a", "Include files starting with dot")
Copy link

@LonMcGregor LonMcGregor Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You offer -a as an option, but is this ever used?

implement-shell-tools/wc/wc.mjs
Copy link

@LonMcGregor LonMcGregor Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout this file there is quite a lot of duplication. Can you think of how you might reduce this?

@LonMcGregor LonMcGregor added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Aug 12, 2025
@Luke-Manyamazi
Refactor shell tools: cat, ls, wc
6da6c6b 
- cat.mjs:
  - Fixed -b option to number only non-blank lines.
  - Unified line number padding to avoid repetition.
  - Added error handling for directories and missing files.

- ls.mjs:
  - Correctly implemented -a option to include hidden files.
  - Validates single directory argument.
  - Sorted output and handled one-column listing.

- wc.mjs:
  - Centralized line, word, and character counting in countFile().
  - Defaults to counting all metrics if no options are given.
  - Added support for multiple files with totals.
  - Improved error handling for missing files and directories.

Overall:
- Removed duplication, clarified logic, and improved alignment with standard Unix behavior.
@Luke-Manyamazi
Copy link
Author

Luke-Manyamazi commented Oct 13, 2025

Hi @LonMcGregor,

Thanks for your feedback! Here’s a summary of the changes and improvements made to cat.mjs, ls.mjs, and wc.mjs:

1️⃣ cat.mjs

Fixed numbering logic for -b (number non-blank lines). Previously, non-blank lines weren’t consistently numbered.
Introduced a clear shouldNumber condition to unify the -n and -b handling.
Pad formatting for line numbers is centralized (padStart(6)) to avoid duplication.
Retains error handling for directories and missing files.

2️⃣ ls.mjs
Ensured the -a option is actually used to include hidden files in the output.
Enforced that only one argument is allowed, mimicking standard ls behavior.
Sorting and one-column (-1) display is maintained.
Added explicit error output when argument count is incorrect.

3️⃣ wc.mjs
Reduced duplication in counting logic by centralizing line, word, and char counts in countFile().
Properly respects which options are passed; defaults to counting all if no option is specified.
Supports multiple files and prints totals when more than one file is processed.
Added error handling for missing files and stdin fallback.

@Luke-Manyamazi Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 13, 2025
LonMcGregor
LonMcGregor reviewed Oct 14, 2025
View reviewed changes
Copy link

@LonMcGregor LonMcGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making these changes. cat and ls look good now. I still notice some duplication in the wc file however.

Have a look at the last 2 blocks of code. They're both doing the same thing: Check options, push to an array,add padding, then print out. Is there a way you could abstract that code a bit?

@LonMcGregor LonMcGregor removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 14, 2025
@Luke-Manyamazi
Refactor wc output formatting - Abstract duplicated code for printing…
c096a0b 
… counts into `formatCounts()` helper - Precompute active count options in `activeCounts` to remove repeated ternaries - Simplify per-file and total output logic for readability and maintainability
@Luke-Manyamazi
Copy link
Author

Luke-Manyamazi commented Oct 14, 2025

Yes, I can abstract that repetition into a helper function, e.g. formatCounts(result, options) that returns the formatted count string for a given result object. Check my implementation.

@Luke-Manyamazi Luke-Manyamazi added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 14, 2025
LonMcGregor
LonMcGregor reviewed Oct 14, 2025
View reviewed changes
Copy link

@LonMcGregor LonMcGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! You're finished with this task now

@LonMcGregor LonMcGregor added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LonMcGregor LonMcGregor LonMcGregor left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Complete Volunteer to add when work is complete and all review comments have been addressed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

01 Implement shell tools (cat, ls, wc) in JavaScript with NodeJS

2 participants

@Luke-Manyamazi @LonMcGregor